-
Notifications
You must be signed in to change notification settings - Fork 308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
glTF 2.0 #67
glTF 2.0 #67
Conversation
@sbtron @bghgary @lexaknyazev @mlimper @cedricpinson @erich666 do you have any input on this approach to convert diffuse/specular materials to PBR metal roughness? See #67 (comment) |
My $0.02, I don't agree with the traditional specular intensity being used as PBR metalness. Traditional diffuse/specular (non-PBR) models never look metallic, so I think it's fine to hard-code metalness to zero for such conversions. The roughness value comes from both specular intensity and shininess. When specular intensity is very low or zero, roughness should be very high, regardless of the shininess setting. When specular intensity is above near-zero, then you have to look at the shininess factor to determine the roughness. I think generally the higher shininess exponents correspond to smoother surfaces, and lower ones to rough surfaces. I would have to play around with some sample models to see what looks visually similar, though. |
The approach you mention is quite lossy. Things will look very poor. We has this discussion some time ago when I was pushing the diffuse-specular instead, but I got voted out. Simple thing to do:
Example code (not finished) in yocto-gl. |
@xelatihy Can you point to a link for the EGSR paper for the conversion you mentioned? I did some experiments with this when we were talking about PBR parameter sets. For context, here is the thread. I would recommend doing what @xelatihy said in 1 and 2, not sure about 3. The conversion from spec-gloss to metal-rough is lossy and none of them are perfect. The live demo of my conversion is here. It is based on the math described here. It's documented in the specs here and here. There is also a note about conversion in the spec-gloss extension appendix. The conversion is not perfect and materials sometimes end up more metallic than they should be. I've been meaning to document how I ended up with the conversion equations, but haven't gotten around to it. Let me know if documenting it will be useful. We have been using this conversion to convert spec-gloss to metal-rough for a few of our models in our testing repo. The Avocado and BarramundiFish are examples of the conversion. Spec-Gloss:
Converted to Metal-Rough:
You can toggle back and forth between the two workflows with the drop-down. |
We had a small discussion about calculating normals KhronosGroup/glTF@fb8b472#commitcomment-21728860. We ended up saying that flat normals will be used if they are not specified in the asset. This is specified in an implementation note in https://github.com/KhronosGroup/glTF/tree/2.0/specification/2.0#meshes |
There's some great discussion on twitter, https://twitter.com/pjcozzi/status/855067965309558784 (also above, #67 (comment)) |
BTW, for the PBR material I should also mention that you should look at the OBJ illum property. |
This is great discussion of the ideal math by @xelatihy and @bghgary. But allow me to offer a completely different point of view, from a typical artist-type user attempting to use this obj2gltf tool. I feel like I have a half-decent understanding of the current workflow for at least some artists who are using OBJ and PBR, after talking to a few in person, watching many online tutorials, and using some of the tools myself to create some sample models (via Collada since this OBJ tool is not yet up to the task). The current workflow goes something like this:
The important thing to understand here is the situation the artist is placed in by the end of step 3: The OBJ still exists, un-modified from step 2. Likewise, some PBR textures now exist, created in step 3 to pair up with the UV map that's baked into the OBJ. But the OBJ doesn't call out the PBR textures by filename, as there are several of those per-material, and they didn't exist yet when the OBJ was created. In fact, in the particular case of a truck model I got from one of our 3D model artists here (Scott), the texturemap name baked into the OBJ is unreadable ("
So for him to use this obj2gltf tool himself, he would need a way to specify the OBJ file, and then specify all eight of these PBR textures, and have the tool understand which texture was assigned to what PBR channel of which material in the OBJ. Plus, the tool would need to disregard the actual texture filename that was specified in the OBJ file itself, as that was just a work-in-progress, non-PBR texture name. The command-line may not be the right approach here. Of course artists have never liked command lines, but in this case it's particularly egregious as the number of files involved increases by 4 per material. I think what artists like Scott need would be more along the lines of a GUI that can load an OBJ, and then allow interactive assignment of textures to PBR channels per material, ideally with a live preview window, and then save the result to glTF 2.0. Actually I have some hope that my vscode extension will someday fill this role, but currently vscode extensions don't have a way to open file dialogs (there's an issue filed) and that puts a crimp in those plans for now. It may be fair to say that interactive assignment of PBR maps is out-of-scope for obj2gltf for now. If that's the case, then some replacement priorities bubble up:
With these qualities, obj2gltf could be used as the initial model load stage of a future interactive tool for assigning PBR textures to glTF files. For users who aren't using PBR, they should just stick to I would much rather see obj2gltf focus itself on enabling professional PBR, and not attempting to up-convert from non-PBR. All of this is just my personal opinion of course :) |
I think this could be supported in the gltf-viewer I've been working on (demo, source). Potential workflow:
I would have a few more questions before trying to implement something like that, probably more appropriate for a separate issue on the glTF repo. In any case, +1 from me on trying to avoid up-conversion to PBR in |
It sounds great to have the flexibility that @emackey suggests (contributions welcome, of course!); however, obj2gltf will still need some reasonable default way to create glTF models without the user having to think about the materials. like today's workflow. This can be built on top of the flexibility, but we are making the barrier-to-entry for glTF too high if we go without it. |
I agree that we need good defaults, and an OBJ going through |
If |
Perhaps we can make this tool align with what COLLADA2GLTF is doing? Rob (@lasalvavida) describes that process in a comment here. |
You can find the C++ implementation for this here. In general: If we have pbrSpecularGlossiness: And then |
Also if anyone has any suggested changes or improvements on this approach I'm happy to hear them; I'm sure many of the people reading this know a ton more about rendering than I do, and may be able to help make this mapping more robust |
Actually, for metal/rough models, the |
Adding @bghgary who has been thinking about this. |
The code is still not 100% ready, but I've pretty much converged on the workflow for this tool. The command line now has three options:
If none of these are supplied, the material is converted from traditional to metallic-roughness PBR. The alternative of passing in metallic/roughness/specular/glossiness values/textures via command line seemed like too much work and maintenance, particularly when multiple materials are involved. This method keeps it simple, but requires that the obj is ready-to-go. As @emackey and @donmccurdy discussed a gui may be more suitable for the task of connecting pbr maps to fix the possibly irrelevant .mtl file. I've decided to keep the traditional->metallic-roughness conversion pretty simple. I started to watch a video about how an artist would convert traditional textures to pbr, and there's too much magic involved to code a proper conversion in here. Along those lines I couldn't find a good way to convert traditional to specular-glossiness, they sound the same but seem to represent completely different concepts. Looking at some sample PBR values here the traditional diffuse would not map well to the specular-glossiness diffuse. So I mainly went with @emackey's suggestion and decided that this tool should just produce good enough values for simple models that are exported at default settings from blender and other programs, but should not be a full blown converter. For example I don't bother converting the specular/shininess textures to PBR because it just feels beyond the scope. The new conversion looks like:
|
@lilleyse This all sounds great. One question though (I haven't looked at the code changes yet), why do you use both And perhaps most importantly, what if the user has a map with combined occlusion (R), roughness (G), and metalness (B) as a single texture already, do you support that? That should be the most common case at least for coming out of Substance Painter (and Unity I think) and going to glTF. |
Right now the assumption is that no textures are packed together. The converter will pack metallic, roughness, and occlusion maps together. |
I'm sure there are situations where that could be useful. But of course "classic" OBJ models don't have these new-fangled PBR maps to start with, so only PBR-aware models would ever need this channel re-packing. Most tools that produce PBR maps will produce them pre-packed, either with configurable mapping or with their own hard-coded mapping. But generally these tools do not produce a pile of loose channels like this in my experience. So having the OBJ converter do this automatically, by default, strikes me as problematic. There should at the very least be an option to enable or disable texture repacking (vs just embedding texture names in the glTF and not loading or altering the textures themselves). Probably this should be off-by-default, as neither classic OBJs nor PBR OBJs typically need this kind of remapping. |
I'm not super familiar with all the exporters, do they generally follow the same packing conventions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's it for part 2.
lib/gltfToGlb.js
Outdated
binaryBuffer = dataUriToBuffer(buffer.uri); | ||
delete buffer.uri; | ||
} else { | ||
binaryBuffer = Buffer.alloc(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that the glb must have external files if the resources are too large to base64 encode? That seems like it should be noted in the README since the glb
spec doesn't seem to have a limit for buffer size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is a weird limitation right now, I'll try fixing it.
module.exports = gltfToGlb; | ||
|
||
/** | ||
* Convert a glTF to binary glTF. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future devs, make note of any assumptions this function makes about the glTF.
lib/loadObj.js
Outdated
}) | ||
.catch(function() { | ||
logger('Could not read image file at ' + imagePath + '. Material will ignore this image.'); | ||
}); | ||
}, {concurrency : 10}) | ||
.thenReturn(images); | ||
.then(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not thenReturn
?
lib/obj2gltf.js
Outdated
* @param {Boolean} [options.metallicRoughness=false] The values in the mtl file are already metallic-roughness PBR values and no conversion step should be applied. Metallic is stored in the Ks and map_Ks slots and roughness is stored in the Ns and map_Ns slots. | ||
* @param {Boolean} [options.specularGlossiness=false] The values in the mtl file are already specular-glossiness PBR values and no conversion step should be applied. Specular is stored in the Ks and map_Ks slots and glossiness is stored in the Ns and map_Ns slots. The glTF will be saved with the KHR_materials_pbrSpecularGlossiness extension. | ||
* @param {Boolean} [options.materialsCommon=false] The glTF will be saved with the KHR_materials_common extension. | ||
* @param {String} [options.metallicRoughnessOcclusionTexture] Path to the metallic-roughness-occlusion texture used by the model, where occlusion is stored in the red channel, roughness is stored in the green channel, and metallic is stored in the blue channel. This may be used instead of setting texture paths in the .mtl file. The model will be saved with a pbrMetallicRoughness material. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the path to XXX texture
parameters disallow materials referencing multiple images in the obj? If so, we should document that users of these "advanced features" need to make sure the input texture covers those channels across the entire input obj.
lib/obj2gltf.js
Outdated
kmcOptions : kmcOptions, | ||
textureCompressionOptions : textureCompressionOptions | ||
|
||
var jsonOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a very slight performance bump, we could set this to undefined when outputting .glb
.
lib/obj2gltf.js
Outdated
return Promise.map(imagePaths, function(imagePath) { | ||
var imageOptions = (imagePath === options.baseColorTexture) ? checkTransparencyOptions : undefined; | ||
return loadImage(imagePath, imageOptions); | ||
}).then(function(images) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Include a concurrency limit in case someone throws an obj with a very large number of textures at this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This place will be ok since it is only loading images passed in above, which would be at most 6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Images that are loaded from the .mtl
file has a concurrency limit.
lib/writeUris.js
Outdated
} | ||
|
||
var source = getBufferPadded(Buffer.concat(sources)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the buffers within sources
also need to be 4-byte aligned? It seems possible for image sources to not be aligned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think they need to be. I forget the exact rules but I think byte alignment is only required for buffer views holding accessor data.
specs/lib/createGltfSpec.js
Outdated
expect(Object.keys(gltf.meshes).length).toBe(3); | ||
expect(gltf.materials.length).toBe(3); | ||
expect(gltf.nodes.length).toBe(4); | ||
expect(gltf.nodes[0].mesh).toBeUndefined(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess for "correctness" we would want this to check the node(s) in gltf.scenes[gltf.scene]
.
specs/lib/createGltfSpec.js
Outdated
}); | ||
}); | ||
|
||
describe('specularGlosiness', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specularGlosiness
-> specularGlossiness
?
@likangning93 I think I address everything except #67 (comment). I'm going to work on that a bit later. |
I just tested this on a bunch of simple OBJs I had lying around. Looks good and nice work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments, and I think I replied to some older comments after the most recent commit. Otherwise, new changes look pretty good.
lib/createGltf.js
Outdated
|
||
if (packMetallic) { | ||
// Write into the B channel | ||
var metallicChannel = getImageChannel(metallicImage, 0, width, height); | ||
writeChannel(pixels, metallicChannel, 2, width, height); | ||
var metallicChannel = getImageChannel(metallicImage, 0, width, height, scratchChannel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's possible that the returned channel is a newly allocated buffer, should this (and below) update scratchChannel
as well? Otherwise this might still cause repeated reallocation in some cases. Seems like it's "fine" if scratchChannel
is bigger than it needs to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scratchChannel
will stay the same size, but sometimes scratchResizeChannel
may need to be re-allocated. I think this is okay given that best practice is to pass textures that are the same dimensions.
lib/createGltf.js
Outdated
var blueChannel = getImageChannel(specularImage, 2, width, height, scratchChannel); | ||
writeChannel(pixels, redChannel, 0); | ||
writeChannel(pixels, greenChannel, 1); | ||
writeChannel(pixels, blueChannel, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be interleaved? Otherwise it seems like this will writeChannel
the blue channel repeatedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops... yes!
lib/loadImage.js
Outdated
info.transparent = hasTransparency(info); | ||
} | ||
return info; | ||
}); | ||
} | ||
return info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return Promise.resolve(info)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored this area a bit so this is no longer a problem.
However, this was okay because nothing was calling .then
on the result, and a promise chain had already started before.
@lilleyse what is the plan here? 😄 |
Still finishing up the tests after reorganizing how materials are loaded. It won't be read by the glTF bof, but is close. |
Rock on and merge when ready! |
@likangning93 this is now ready for final review. I got a bit carried away and went on a cleaning/reorganization spree. Sorry about the large diff yet again. I believe it is final The major changes since before are:
|
Doesn't have to be this PR, but eventually we should support a stream-based implementation so that you simply write objects to a user-supplied stream, that way you could handle multi-file output without having to write to a specific directory (think DB import) |
@likangning93 merged in master, all ready now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more comments, mostly about future stuff or documentation.
return gltf; | ||
} | ||
|
||
function addBufferView(gltf, buffers, accessors, byteStride, target) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't have to be in this PR, but just since there's this many parameters and it seems like there's some assumptions being made, it might be nice for future devs if there's a private JSDoc.
|
||
// Add meshes to node | ||
var meshes = node.meshes; | ||
var meshes = nodes[i].meshes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably should be done in a separate PR, but should this check be per-mesh instead of over the whole asset? I guess the added complexity of supporting different types of indices within one gltf might mess with buffer/bufferView code in here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I used to do before I realized Cesium does not like different index types being in the same buffer view. I think the situation has changed a bit since different sized indices wouldn't be allowed in the same bufferView anyways in gltf 2.0.
I do like the simplicity of this approach though, but I'll open an issue since I can think of some models that would require more memory because of this.
if (argv.metallicRoughness + argv.specularGlossiness + argv.materialsCommon > 1) { | ||
console.error('Only one material type may be set from [--metallicRoughness, --specularGlossiness, --materialsCommon].'); | ||
process.exit(1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like these checks duplicate what's in lib/obj2gltf
, I guess is this because we don't want CLI users to see DeveloperErrors
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that was my only reason.
var separateTextures = options.separateTextures; | ||
|
||
var promises = []; | ||
if (separateTextures) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also check separate
? Looks like otherwise images end up in the external buffer. If that's intended behavior we should probably document it since it's new to 2.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's a bit hard to notice, but if separate
is true separateTextures
will also be true.
In obj2gltf.js
options.separateTextures = defaultValue(options.separateTextures, defaults.separateTextures) || options.separate;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok.
} | ||
} | ||
|
||
function encodeTextures(gltf) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something for a later PR, can we determine cases where the alpha channel isn't needed and encode to JPEG instead? Seems like something that users will want for smaller assets (spec permitting).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #100
options.packOcclusion = true; | ||
|
||
var material = loadMtl._createMaterial({ | ||
ambientTexture : ambientTexture, // Is a .gif which can't be decoded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like there's any special handling in loadTexture
if a gif
source is given, should there be a warning to the dev/user? Similar to places where textures can't be found or a default is substituted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning is provided later, but only if the PBR texture is created. Otherwise it will just ignore that it couldn't be decoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Since these are warnings instead of early-fails, this seems fine but might warrant a little documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added doc to loadMtl
.
Thanks @lilleyse! |
This has initial support for gltf 2.0.
I would like some input on my approach on converting traditional diffuse/specular materials to pbr metallic-roughness materials. These are the basic steps:
Also I have one comment left in the code that I'm wondering about. Is there a convention for lighting models when they don't contain normals? Should
pbrMetallicRoughness
be leftundefined
and let the implementation figure it out? Right now I do keep the pbr material but set all the values to either black or 0.0, and set the emissive color instead. I'm wondering what different glTF 2.0 viewers might do with a material like this.To-do: